Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add short const support, make constant scraping impl. testable #1503

Merged
merged 4 commits into from
Mar 22, 2023
Merged

Add short const support, make constant scraping impl. testable #1503

merged 4 commits into from
Mar 22, 2023

Conversation

riverar
Copy link
Collaborator

@riverar riverar commented Mar 18, 2023 •
edited
Loading

Fixes: #1029

Added support for short PSTR/PWSTR constant values in special cases (e.g. MAKEINTRESOURCE). Did some light refactoring to the constant parsing code to get some unit testing going. More refactor work is needed; over time we'll eliminate our need for the abstractions dependency.

New ConstantsScraper dependencies:

  • System.IO.Abstractions

New test dependencies:

  • System.IO.Abstractions
  • System.IO.Abstractions.TestingHelpers
  • Moq
  • MSTest.TestFramework

@mikebattista Can you help with the winmd changes comparison script? It doesn't seem to be picking up on the subtle differences here.

@mikebattista
Copy link
Contributor

I thought everyone agreed that we shouldn't be changing the types of these to short. See #1029 (comment).

@riverar
Copy link
Collaborator Author

riverar commented Mar 20, 2023

Here's what the output previously looked like:

.field public static literal valuetype Windows.Win32.Foundation.PWSTR TD_WARNING_ICON = int32(-1)

And how it looks like afterwards:

.field public static literal valuetype Windows.Win32.Foundation.PWSTR TD_WARNING_ICON = int16(-1)

@mikebattista
Copy link
Contributor

Ok. Can you update the baseline?

@riverar
Copy link
Collaborator Author

riverar commented Mar 20, 2023

@mikebattista That's what I was asking you for help with (bottom of PR) 🙃. Doesn't look like it's detecting the changes. If you don't get to it, no worries, I can take a look off work hours.

@riverar
Copy link
Collaborator Author

riverar commented Mar 20, 2023

Tweaked the PR text as I realized it was causing some confusion (short constants vs short pwstr constant values). Sorry about that.

@mikebattista
Copy link
Contributor

My guess is the below needs to change. GetConstantValue and ToString likely aren't enough to detect the change.

win32metadata/sources/WinmdUtils/Program.cs

Lines 1129 to 1148 in 48d738c

var fieldVal1 = field1.GetConstantValue();
var fieldVal2 = field2.GetConstantValue();
if (fieldVal1 == null)
{
writer.WriteDifference($"winmd1: {fullField1Name} is a constant with a null value");
}
if (fieldVal2 == null)
{
writer.WriteDifference($"winmd2: {fullField1Name} is a constant with a null value");
}
string val1 = fieldVal1?.ToString();
string val2 = fieldVal2?.ToString();
if (val1 != val2)
{
writer.WriteDifference($"winmd1: {fullField1Name} = {val1}, winmd2 = {val2}");
}

@riverar
Copy link
Collaborator Author

riverar commented Mar 22, 2023

New diff:

Windows.Win32.Media.KernelStreaming.Apis.RT_RCDATA value init changed Int32->Int16
Windows.Win32.Media.KernelStreaming.Apis.RT_STRING value init changed Int32->Int16
Windows.Win32.UI.Controls.Apis.TD_ERROR_ICON value init changed Int32->Int16
Windows.Win32.UI.Controls.Apis.TD_INFORMATION_ICON value init changed Int32->Int16
Windows.Win32.UI.Controls.Apis.TD_SHIELD_ICON value init changed Int32->Int16
Windows.Win32.UI.Controls.Apis.TD_WARNING_ICON value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_APPSTARTING value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_ARROW value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_CROSS value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_HAND value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_HELP value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_IBEAM value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_ICON value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_NO value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_PERSON value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_PIN value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZE value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZEALL value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZENESW value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZENS value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZENWSE value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_SIZEWE value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_UPARROW value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.IDC_WAIT value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_ACCELERATOR value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_ANICURSOR value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_ANIICON value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_BITMAP value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_CURSOR value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_DIALOG value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_DLGINCLUDE value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_FONT value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_FONTDIR value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_HTML value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_ICON value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_MENU value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_MESSAGETABLE value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_PLUGPLAY value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_VERSION value init changed Int32->Int16
Windows.Win32.UI.WindowsAndMessaging.Apis.RT_VXD value init changed Int32->Int16

mikebattista
mikebattista previously approved these changes Mar 22, 2023
View reviewed changes
@mikebattista mikebattista merged commit 30a14ca into microsoft:main Mar 22, 2023
@mikebattista
Copy link
Contributor

Awesome. Thanks!

@riverar riverar deleted the rafael/makeintresource branch March 23, 2023 16:59
@riverar riverar mentioned this pull request Mar 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikebattista mikebattista mikebattista left review comments

@chenss3 chenss3 Awaiting requested review from chenss3

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MAKEINTRESOURCE integers should be 16-bit not 32-bit
2 participants
@riverar @mikebattista